This doesn't really make sense to me. If file mode puts one .proto file in each proto_library target, it doesn't seem meaningful to group them at all. Why would proto_group have any effect?

It was mostly a side-effect of the way I implemented the per-file option, but I think it's pretty useful. The proto_group option allows you to override the default name of the proto_library rule. If for some reason someone wanted to logically group proto files, but still default to single proto file libraries, this allows that.

You could also use this option to rename a proto_library, if that was desired for whatever reason.

I think I could improve my wording here. How about:

The proto_library a .proto file ends up in can be overridden by using the proto_group option.

The proto_group option allows you to override the default name of the proto_library rule.

I wonder if a better way to do that in this mode would be to match on srcs when finding targets to merge? In language/proto/kinds.go, it looks like proto_library will already do that. That seems a much more intuitive workflow: users can just change the name of the target in the build file, and Gazelle keeps using the new name thereafter. proto_group seems much less intuitive to me for this use case.

If for some reason someone wanted to logically group proto files, but still default to single proto file libraries, this allows that.

Gazelle's probably a poor fit for that level of customization. I'd recommend manually maintaining build files for directories like this.

In any case, if proto_group does this, it needs to be tested to make sure it doesn't break later on.

Thanks for bringing this up. After doing some more digging I found that the proto_group method didn't even work in file mode 🤦 (else if vs if). Untested code is broken code... Anyway, I dropped the docs about it and the (faulty) code that would have made it work, as suggested.

I was unaware of MatchAttrs, that should handle most use cases that I was thinking about.

Does it look good now?

@jayconrod have you had a chance to look at this?

Regrettably, I no longer have bandwidth to work on Gazelle. I'll add @bazelbuild/go-maintainers in case anyone else can take a look.

@achew22 are you able to take a quick look at this?

Can you have one of these import from the other to confirm that the dep detection logic works

I have added an import statement to this file, which could test that I haven't broken the parsing logic, but fwiw these tests are just for the generate behavior, not resolve. I haven't made any changes to the resolve behavior, which is tested separately.

Right now I don't believe that resolve is affected by mode changes at all. I could loop through all the available modes on the config here to test that the existing tests pass with all modes? https://github.com/bazelbuild/bazel-gazelle/blob/b1e37463c2ef0997c5cf0e90cdf03e696caaed27/language/proto/resolve_test.go#L357

What do you think?

Re: your comment in Slack

I'm not concerned about it changing the result mode, I want to make sure that it actually does the resolve correctly when there is single file target generation

I've added a simple test in 8c77987 that imports a single proto given an index of three different protos with separate proto_librarys.

The previous test that I added for the comment above has been removed.

Should be good now? @achew22

Shouldn't this have a deps entry for ":bar_proto"?

Hmm, I don't think so. The resolve tests are entirely separate from the generate tests, is that where the confusion is from?

I think I should remove the import there (just did so in a5bd0e0), as I think it's just confusing.

I have added an import statement to this file, which could test that I haven't broken the parsing logic, but fwiw these tests are just for the generate behavior, not resolve. I haven't made any changes to the resolve behavior, which is tested separately.

To be clear, it was possible to have many different proto_library packages in a directory before, but gazelle wouldn't auto-generate them for you, you would need to use different packages (in the same dir) or an option in the proto file that defined a grouping behavior (with appropriate gazelle config changes). The resolve behavior for single proto file proto_librarys should have worked before and after this change regardless of the change to the generate.go behavior.

Anyway, the new entry in resolve_test.go should be what you're asking for?

As it stands there isn't an end-to-end test for the proto language plugin. I have thought about adding something like it, but it would be a larger change and deserve it's own PR. Basic thought would be to have the testdata directory, but instead of BUILD.old and BUILD.want, you'd have BUILD.old, BUILD.generate, BUILD.resolve, (and maybe BUILD.merge?). I would need to dig into it a bit more, but it would be refactoring all of the existing tests in this plugin (and maybe also for the go plugin).

As a counterpoint, adding a new stage for those tests might be redundant. The resolve_test.go file defines a similar structure that isn't backed by .proto source files, and it can create arbitrarily complex situations for the resolver that may not be output by generate.go.